Skip to content

[native] Fix MV mapping#24787

Merged
zation99 merged 1 commit intoprestodb:masterfrom
zation99:fix_mv_mapping_handler_type
Mar 24, 2025
Merged

[native] Fix MV mapping#24787
zation99 merged 1 commit intoprestodb:masterfrom
zation99:fix_mv_mapping_handler_type

Conversation

@zation99
Copy link
Contributor

@zation99 zation99 commented Mar 24, 2025 •
edited
Loading

Description

The original approach will cause a "Unsupported table writer handle" error. We should use InsertHandle with the MaterializedViewHandle key.
The order also needs to have MaterializedViewHandle key before InsertHandle key, otherwise the insertHandle type will be replaced with MaterializedViewHandle.

Motivation and Context

Impact

Test Plan

refresh materialized view query

Contributor checklist

  • Please make sure your submission complies with our contributing guide, in particular code style and commit standards.
  • PR description addresses the issue accurately and concisely. If the change is non-trivial, a GitHub Issue is referenced.
  • Documented new properties (with its default value), SQL syntax, functions, or other functionality.
  • If release notes are required, they follow the release notes guidelines.
  • Adequate tests were added if applicable.
  • CI passed.

Release Notes

Please follow release notes guidelines and fill in the release notes below.

== NO RELEASE NOTE ==

@zation99 zation99 requested a review from a team as a code owner March 24, 2025 01:10
@prestodb-ci prestodb-ci added the from:Meta PR from Meta label Mar 24, 2025
@aditi-pandit aditi-pandit changed the title [fix]Fix MV mapping [native] Fix MV mapping Mar 24, 2025
@aditi-pandit
Copy link
Contributor

aditi-pandit commented Mar 24, 2025

@zation99 : Is this the only change required for RefreshMaterializedView ? Does MaterializedViewHandle map to InsertTableHandle and then the rest of Velox code work as is for Materialized Views ? If yes, then would be great if you add an e2e test in https://github.com/prestodb/presto/blob/master/presto-native-execution/src/test/java/com/facebook/presto/nativeworker as well.

gggrace14
gggrace14 approved these changes Mar 24, 2025
View reviewed changes
Copy link
Contributor

@gggrace14 gggrace14 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Prestissimo. Nice!

@zation99 zation99 merged commit 21bdd48 into prestodb:master Mar 24, 2025
98 checks passed
@zation99
Copy link
Contributor Author

zation99 commented Mar 24, 2025

@zation99 : Is this the only change required for RefreshMaterializedView ? Does MaterializedViewHandle map to InsertTableHandle and then the rest of Velox code work as is for Materialized Views ? If yes, then would be great if you add an e2e test in https://github.com/prestodb/presto/blob/master/presto-native-execution/src/test/java/com/facebook/presto/nativeworker as well.

Oh sorry I missed your comment. Yes that's all needed and Velox uses InsertHandle for this.
Let me add an e2e test in a follow-up PR.

@prestodb-ci prestodb-ci mentioned this pull request Mar 28, 2025
Merged
30 tasks
pradeepvaka pushed a commit to pradeepvaka/presto that referenced this pull request Apr 11, 2025
@zation99 @pradeepvaka
Fix MV mapping (prestodb#24787)
f394813 
The original approach will cause a "Unsupported table writer handle" error. We should use InsertHandle with the MaterializedViewHandle key.
The order also needs to have MaterializedViewHandle key before InsertHandle key, otherwise the insertHandle type will be replaced with MaterializedViewHandle.
@unidevel unidevel mentioned this pull request Apr 25, 2025
Closed
30 tasks
This was referenced May 6, 2025
Closed
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@gggrace14 gggrace14 gggrace14 approved these changes

@jainxrohit jainxrohit Awaiting requested review from jainxrohit

@amitkdutta amitkdutta Awaiting requested review from amitkdutta

@aditi-pandit aditi-pandit Awaiting requested review from aditi-pandit

Assignees

No one assigned

Labels

from:Meta PR from Meta

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

4 participants

@zation99 @aditi-pandit @gggrace14 @prestodb-ci